Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance Etcd configMap to provide consistency and controllability #812

Merged
merged 8 commits into from
Nov 26, 2024

Conversation

anveshreddy18
Copy link
Contributor

@anveshreddy18 anveshreddy18 commented Jun 24, 2024

How to categorize this PR?

/area quality
/kind cleanup
/kind enhancement

What this PR does / why we need it:

This PR is to cleanup the Etcd ConfigMap to :

  • Rename the configMap in the format {etcd.Name}-config instead of etcd-bootstrap-<UID>. This is for consistency with other resources that gets created as part of Etcd Reconciliation, like leases, role, rolebinding, etc and also have a proper name config for the configMap instead of bootstrap. The cleanup logic for the existing configMap with older name will be taken up at a later stage after 2 releases. This issue tracks that.
  • Make snapshot-count configurable with the flag snapshotCount from spec.etcd section of etcd CR thus the operator is able to control the no.of transactions before committing a snapshot to disk if they want to.
  • Use proper URLs for initial-advertise-peer-urls and advertise-client-urls properties of the configMap and thereby fixing [Enhancement] Simplify etcd configuration and use proper URL instead of using @ as separator #476

Previously the fields initial-advertise-peer-urls and advertise-client-urls of the etcd ConfigMap were constructed using an @ separator in the format scheme@peerSvcName@namespace@port which is then parsed by the backup-restore container to construct the full URL to be used by the etcd container as its configuration.

This PR aims to modify that to have the fully qualified URL(s) for each etcd member in the ConfigMap itself which then backup-restore parses and sends only it's corresponding etcd member's URL(s) to the etcd process.

The following is the format in the ConfigMap for the fields initial-advertise-peer-urls & advertise-client-urls.

initial-advertise-peer-urls:
  etcd-main-0:
    - main in-cluster url
    - additional urls if required (e.g. live CPM)
  etcd-main-1:
    - main in-cluster url
    - additional urls if required (e.g. live CPM)
  etcd-main-2:
    - main in-cluster url
    - additional urls if required (e.g. live CPM)
advertise-client-urls:
  etcd-main-0:
    - main in-cluster url
    - additional urls if required (e.g. live CPM)
  etcd-main-1:
    - main in-cluster url
    - additional urls if required (e.g. live CPM)
  etcd-main-2:
    - main in-cluster url
    - additional urls if required (e.g. live CPM)

The additional urls above is used for e.g. in CPM in the context of Gardener use case. For now from code we only populate the main in-cluster URL, but in future we will modify the API to allow passing these additional urls from the Etcd Spec which will get populated here and be subsequently parsed and sent to etcd process.

Which issue(s) this PR fixes:
Fixes #717

Special notes for your reviewer:

Release note:

Enhances Etcd configuration by organizing ConfigMap naming convention, enabling snapshot-count configuration, and rectifying URL issues for improved functionality and consistency
Etcd ConfigMap Naming Update: The naming convention has changed to {etcd.Name}-config for consistency, replacing etcd-bootstrap-<UID>. Unused old ConfigMaps will be removed in v0.27.0

@anveshreddy18 anveshreddy18 requested a review from a team as a code owner June 24, 2024 11:05
@gardener-robot gardener-robot added needs/review Needs review area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/cleanup Something that is not needed anymore and can be cleaned up kind/enhancement Enhancement, improvement, extension size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jun 24, 2024
@anveshreddy18 anveshreddy18 self-assigned this Jun 24, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 24, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 24, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 25, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 25, 2024
@gardener-robot gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Sep 2, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 2, 2024
@anveshreddy18
Copy link
Contributor Author

/assign @shreyas-s-rao
/hold

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Sep 5, 2024
@renormalize
Copy link
Member

renormalize commented Sep 24, 2024

@anveshreddy18 can you please add gardener/etcd-backup-restore#715 merge and release as a prerequisite to this PR's description? Just so we don't accidentally merge this PR before the changes are etcd-backup-restore are released.

@anveshreddy18
Copy link
Contributor Author

@anveshreddy18 can you please add gardener/etcd-backup-restore#715 merge and release as a prerequisite to this PR's description? Just so we don't accidentally merge this PR before the changes are etcd-backup-restore are released.

Sure, I hope your comment itself is sufficient. But I can add that once this PR is reviewed as we may lose any comment that we put right now because of the reviews. And for now the do-not-merge label is present, I will make sure to keep that label till etcdbr PR is merged and released.

@shreyas-s-rao shreyas-s-rao added this to the v0.24.0 milestone Oct 8, 2024
@gardener-robot gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Oct 8, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 8, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 8, 2024
@shreyas-s-rao
Copy link
Contributor

/retest

@seshachalam-yv
Copy link
Contributor

@anveshreddy18 rebase with master

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 26, 2024
@anveshreddy18
Copy link
Contributor Author

/test pull-etcd-druid-integration

@anveshreddy18
Copy link
Contributor Author

/retest

Copy link

gardener-prow bot commented Nov 26, 2024

@anveshreddy18: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-etcd-druid-e2e-kind-nondistroless-etcd 5ec1735 link true /test pull-etcd-druid-e2e-kind-nondistroless-etcd

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@shreyas-s-rao
Copy link
Contributor

e2e test pull-etcd-druid-e2e-kind-nondistroless-etcd is expected to fail, since the configmap URL parsing has not been updated for etcd-backup-restore:v0.24.x release branch. This is expected, since support for non-distroless images will be removed by #936 in this release of etcd-druid.

Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anveshreddy18 thanks for making the requested changes.
/lgtm

@gardener-robot gardener-robot added the reviewed/lgtm Has approval for merging label Nov 26, 2024
@gardener-robot gardener-robot removed needs/changes Needs (more) changes needs/review Needs review labels Nov 26, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 26, 2024
@seshachalam-yv seshachalam-yv merged commit 06080a2 into gardener:master Nov 26, 2024
13 of 14 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/cleanup Something that is not needed anymore and can be cleaned up kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

☂️ Etcd ConfigMap changes
10 participants